-
Notifications
You must be signed in to change notification settings - Fork 27
Add 'translation_strategy' to PrometheusMetricExporter #262
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Add 'translation_strategy' to PrometheusMetricExporter #262
Conversation
Signed-off-by: Arthur Silva Sens <[email protected]>
The big doubt here is how to proceed with deprecating the existing configuration. The OTel collector already uses the PrometheusMetricExporter schema, so removing those options would mean breaking the Collector configuration. At the same time, the PrometheusExporter schema is marked as experimental... so not sure if there is a concept of "Deprecation" here. |
schema/type_descriptions.yaml
Outdated
|
||
Deprecated: Use .translation_strategy instead. | ||
without_type_suffix: > | ||
Configure Prometheus Exporter to produce metrics without a type suffix. | ||
|
||
If omitted or null, false is used. | ||
|
||
Deprecated: Use .translation_strategy instead. | ||
without_scope_info: > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do these get interpreted if they are supplied with a defined translation_strategy
?
Should they just be removed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's something I'd love to discuss. The OTel Collector is already using those fields in their configuration files; if we simply remove them, it would be a breaking change for the collector.
It would be nice to keep both for a while, while we announce the deprecation in the collector for a few releases. In the meantime, I imagine the translation_strategy
will always take precedence over the two deprecated fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
Can we document the precedence of translation_strategy
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Per the OTEL spec SIG, we can mark as deprecated but cannot reasonably remove the old options. We can make sure to emphasize the new options in docs etc, but will have to keep supporting the old code ~forever. |
…ptions Signed-off-by: Arthur Silva Sens <[email protected]>
Hmmm, I'm not sure if the CI failures are related to my changes. I'm assuming they are not 🤔 |
Fixes #261
This PR adds a new configuration option to control how PrometheusMetricExporters expose Prometheus metrics—complying with the spec change open-telemetry/opentelemetry-specification#4533. This new option is meant to replace the already existing options
without_units
andwithout_type_suffix
, while adding extra control over whether UTF8 characters should be exposed or replaced with underscores.